module: add tests for two cases of eager imports:#64197
Conversation
nodejs-github-bot
added
esm
There was a problem hiding this comment.
Note that this is already tested in test262:
- https://github.com/tc39/test262/tree/main/test/language/import/import-defer/evaluation-top-level-await/import-defer-async-module
- https://github.com/tc39/test262/blob/main/test/language/import/import-defer/evaluation-sync/module-imported-defer-and-eager.js
I am not super familiar with the Node.js implementation of ESM, but test262 is run through V8's d8.
Whether or not it makes sense to have these two tests here probably depends on how much they are testing V8-internal code paths vs if there is any part of it that is implemented on the Node.js side.
The tests look correct :)
|
Oh, could you update the commit first line to mention that it's about import defer? |
|
This doesn't look very Node.js specific and should live in test262 instead? As long as the V8 side works alright I think it's unlikely that anything in particular in Node.js would make it not work. |
MayaLekova
force-pushed
the
import-defer-02
branch
from
af6b72a to
87536da
Compare
I was thinking about it and as those 2 tests are really simple, I think it's worth having them in Node.js as well. They are not anymore engine-specific than the initial 2 smoke tests I added in Node. Also makes it really easy to run and - if needed - debug the code in Node directly. |
MayaLekova
force-pushed
the
import-defer-02
branch
3 times, most recently
from
78d8282 to
b135b16
Compare
- when the defer imported module contains top-level await - when the defer imported module is also eagerly imported Refs: https://github.com/tc39/proposal-defer-import-eval Signed-off-by: Maya Lekova <maya@igalia.com>
MayaLekova
force-pushed
the
import-defer-02
branch
from
b135b16 to
ebce535
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64197 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.77%
==========================================
Files 379 741 +362
Lines 166972 240745 +73773
Branches 25554 45354 +19800
==========================================
+ Hits 153639 217270 +63631
- Misses 13041 15048 +2007
- Partials 292 8427 +8135 🚀 New features to boost your workflow:
|
|
Thanks for looking into this! |
joyeecheung
left a comment
joyeecheung
left a comment
There was a problem hiding this comment.
I think two small tests are okay to add if they make debugging easier, though we should be cautious about duplicating tests for cases already covered in the upstream, especially when the V8 version on main tends to be months behind the upstream and the proposal can still go through semantic changes..
github-actions
Bot
removed
the
request-ci
joyeecheung
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in a006262 |
5 participants
module: add tests for two cases of eager imports when using
import deferRefs: https://github.com/tc39/proposal-defer-import-eval
Signed-off-by: Maya Lekova maya@igalia.com